Fix GH-20214: PDO::FETCH_DEFAULT unexpected behavior with setFetchMode#21434
Fix GH-20214: PDO::FETCH_DEFAULT unexpected behavior with setFetchMode#21434iliaal wants to merge 4 commits intophp:PHP-8.4from
Conversation
iliaal
force-pushed
the
fix/gh-20214-pdo-fetch-default
branch
from
40d5d5a to
72d1b29
Compare
SakiTakamachi
left a comment
SakiTakamachi
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
I’ve left a few minor comments, but I agree with the overall approach.
| if ((mode & ~PDO_FETCH_FLAGS) == PDO_FETCH_USE_DEFAULT) { | ||
| mode = stmt->dbh->default_fetch_type; | ||
| } |
There was a problem hiding this comment.
These need to be placed after flags = mode & PDO_FETCH_FLAGS;.
Otherwise, when flags such as PDO_FETCH_GROUP are specified, they will be overwritten with 0.
| @@ -0,0 +1,38 @@ | |||
| --TEST-- | |||
There was a problem hiding this comment.
This test should be included in PDO core rather than pdo_sqlite.
If you write it with reference to tests like those in ext/pdo/tests/pdo_xxx.phpt, it will be executed across all drivers.
Also, pdo_stmt_setup_fetch_mode is used not only by setFetchMode, but also when executing query with a second argument.
Therefore, I recommend adding test cases for those scenarios as well.
It would be even better if you can verify that flags such as PDO_FETCH_GROUP are not being overwritten.
|
Good catches. The flags issue was a real bug. I moved the resolution after flags extraction so caller-supplied flags like flags = mode & PDO_FETCH_FLAGS;
if ((mode & ~PDO_FETCH_FLAGS) == PDO_FETCH_USE_DEFAULT) {
mode = stmt->dbh->default_fetch_type | flags;
}Moved the test to |
|
@iliaal |
…ment::setFetchMode When setFetchMode(PDO::FETCH_DEFAULT) is called, mode=0 (PDO_FETCH_USE_DEFAULT) gets stored as the statement's default fetch type. Later, do_fetch() tries to resolve PDO_FETCH_USE_DEFAULT by reading stmt->default_fetch_type, which is also 0 — circular reference that on 8.4 silently fell through to FETCH_BOTH and on master throws a ValueError. Resolve PDO_FETCH_USE_DEFAULT to the connection-level default early in pdo_stmt_setup_fetch_mode(), before flags extraction and the mode switch, so the rest of the function processes the actual fetch mode. Closes phpGH-20214
Extract flags before resolving PDO_FETCH_USE_DEFAULT so caller-supplied flags (e.g. PDO_FETCH_GROUP) are preserved. Move test from pdo_sqlite to ext/pdo/tests/ for cross-driver coverage. Add test cases for query() with second argument and flags preservation.
Firebird requires SELECT ... FROM table syntax; bare SELECT 'val' AS col is not valid. Create a test table to make the test cross-driver compatible.
iliaal
requested review from
Girgias,
TimWolla,
adoy,
arnaud-lb,
derickr,
devnexen,
dstogov,
kocsismate and
ndossche
as code owners
iliaal
force-pushed
the
fix/gh-20214-pdo-fetch-default
branch
from
2d59faa to
40868e4
Compare
@SakiTakamachi I ported the code to 8.4 all the PDO tests pass, however FreeBSD seems to abort with infra error and in other environments un-related ssl (flaky??) tests seem to be failing which are not at all related to this change. |
2 participants
When
setFetchMode(PDO::FETCH_DEFAULT)is called,pdo_stmt_setup_fetch_mode()storesPDO_FETCH_USE_DEFAULT(0) as the statement's default fetch type. Later,do_fetch()tries to resolvePDO_FETCH_USE_DEFAULTby readingstmt->default_fetch_type— which is also 0, creating a circular reference. On 8.4 this silently fell through toFETCH_BOTH; on master it throws aValueError.The fix resolves
PDO_FETCH_USE_DEFAULTto the connection-level default (stmt->dbh->default_fetch_type) early inpdo_stmt_setup_fetch_mode(), before flags extraction and the mode switch. The connection default is guaranteed to never bePDO_FETCH_USE_DEFAULT(enforced byPDO::setAttribute), so no circular resolution is possible.Not sure if this should be rebased to the PHP-8.4 branch since the bug affects 8.4 as well (silently returns wrong fetch mode there).
Fixes #20214